fix: correctly fail geocoding jersey postcodes #273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the postcode geocoding functionality where Jersey postcodes (and potentially other postcodes) could return responses from the postcodes.io API with null values for postcode, latitude, or longitude fields, causing the application to crash or behave incorrectly instead of gracefully failing.
Changes:
- Added additional validation checks to ensure
postcode,latitude, andlongitudefields are not null/falsy before processing - Added a test case with a Jersey postcode (JE3 2DF) to the test data file to validate the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/server/mapping/geocode.ts | Added null/falsy value checks for postcode, latitude, and longitude fields in postcodes.io API response validation |
| tests/resources/members.csv | Added Notes column and Jersey postcode test case; also added notes to existing test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| George,G115RD, | ||
| Ringo,HP20 2QB, | ||
| Jane,BT15 3ES,Test Belfast | ||
| Mysterion,JE3 2DF, Test Jersey (currently does not geocode as is missing in the database and postcodes.io) |
There was a problem hiding this comment.
There is an extra space after the comma before 'Test Jersey'. This should be 'JE3 2DF,Test Jersey' to be consistent with line 6 where 'BT15 3ES,Test Belfast' has no space after the comma.
| !postcodesData.result.latitude || | ||
| !postcodesData.result.longitude |
There was a problem hiding this comment.
These checks will fail for valid latitude/longitude values of 0. For example, postcodes on the Prime Meridian have longitude 0, and postcodes on the Equator have latitude 0. Consider using explicit null/undefined checks instead: postcodesData.result.latitude == null || postcodesData.result.longitude == null.
| !postcodesData.result.latitude || | |
| !postcodesData.result.longitude | |
| postcodesData.result.latitude == null || | |
| postcodesData.result.longitude == null |
| George,G115RD | ||
| Ringo,HP20 2QB | ||
| Jane,BT15 3ES | ||
| Name,Postcode,Notes |
There was a problem hiding this comment.
The test expectations in tests/feature/importDataSource.test.ts still expect only 5 records (lines 81-171), but adding a 7th row to members.csv means the test will now import 6 records. The test expectations need to be updated to handle the new Jersey postcode record, or the test should explicitly verify that the Jersey record is excluded due to geocoding failure.
joaquimds
force-pushed
the
fix/jersey-postcodes
branch
from
5c815f6 to
4cd9427
Compare
No description provided.